Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update helm/docs per changes in supported task discovery #5694

Merged
merged 3 commits into from
Sep 3, 2024
Merged

Update helm/docs per changes in supported task discovery #5694

merged 3 commits into from
Sep 3, 2024

Conversation

Sovietaced
Copy link
Contributor

@Sovietaced Sovietaced commented Aug 26, 2024 •
edited
Loading

Why are the changes needed?

These changes aren't strictly needed but they simplify configuration based on the changes introduced in #5460

What changes were proposed in this pull request?

Removes explicit configuration supported task types since they are now automatically discovered.

How was this patch tested?

Built and ran the sandbox bundled. Executed a file sensor workflow.

Screenshot 2024-08-30 at 5 06 03 PM

Seen in the logs..

{"json":{"exec_id":"akz742lxljtfn2dk6r7j","node":"n0","ns":"flytesnacks-development","res_ver":"2061","routine":"worker-12","src":"handler.go:180","wf":"flytesnacks:development:sensor.wf"},"level":"info","msg":"Dynamic handler.Handle's called with phase 0.","ts":"2024-08-31T00:03:47Z"}
{"json":{"exec_id":"akz742lxljtfn2dk6r7j","node":"n0","ns":"flytesnacks-development","res_ver":"2061","routine":"worker-12","src":"handler.go:344","tasktype":"sensor","wf":"flytesnacks:development:sensor.wf"},"level":"debug","msg":"Plugin [agent-service] resolved for Handler type [sensor]","ts":"2024-08-31T00:03:47Z"}

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

@Sovietaced Sovietaced force-pushed the supported-tasks-cleanup branch from eac6e67 to db28350 Compare August 26, 2024 19:12
@codecov Codecov
Copy link

codecov bot commented Aug 26, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.17%. Comparing base (7a91799) to head (cefad53).
Report is 155 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5694      +/-   ##
==========================================
- Coverage   36.17%   36.17%   -0.01%     
==========================================
  Files        1303     1303              
  Lines      109663   109663              
==========================================
- Hits        39667    39666       -1     
- Misses      65851    65852       +1     
  Partials     4145     4145
Flag Coverage Δ
unittests-datacatalog 51.37% <ø> (ø)
unittests-flyteadmin 55.28% <ø> (ø)
unittests-flytecopilot 12.17% <ø> (ø)
unittests-flytectl 62.18% <ø> (ø)
unittests-flyteidl 7.12% <ø> (ø)
unittests-flyteplugins 53.34% <ø> (ø)
unittests-flytepropeller 41.76% <ø> (ø)
unittests-flytestdlib 55.33% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sovietaced Sovietaced marked this pull request as ready for review August 26, 2024 19:21
@neverett
Copy link
Contributor

Pinging @davidmirror-ops since he has more expertise in the helm charts & deployment docs than me or Peeter.

Future-Outlier
Future-Outlier reviewed Aug 27, 2024
View reviewed changes
Copy link
Member

@Future-Outlier Future-Outlier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's some todo:

  1. solve the merged conflict
  2. before we merge it, we have to build an image of flyte and test it.
    go to this folder and run
    make build
    https://github.com/flyteorg/flyte/tree/master/docker/sandbox-bundled

if possible, please run a sensor task and provide screenshot that new image works!
https://docs.flyte.org/en/latest/flytesnacks/examples/sensor/file_sensor_example.html

@Sovietaced Sovietaced force-pushed the supported-tasks-cleanup branch from bda86fd to f1ecfd9 Compare August 30, 2024 23:01
@Sovietaced
Copy link
Contributor Author

Sovietaced commented Aug 31, 2024 •
edited
Loading

here's some todo:

  1. solve the merged conflict
  2. before we merge it, we have to build an image of flyte and test it.
    go to this folder and run
    make build
    https://github.com/flyteorg/flyte/tree/master/docker/sandbox-bundled

if possible, please run a sensor task and provide screenshot that new image works! https://docs.flyte.org/en/latest/flytesnacks/examples/sensor/file_sensor_example.html

I have updated the PR description and seems that I was able to get the task running, but not sure how to satisfy the sensor so the task completes. But I can verify that propeller identifies the agent-service for the task type.

@Sovietaced Sovietaced force-pushed the supported-tasks-cleanup branch 2 times, most recently from 6f52211 to fa18557 Compare August 31, 2024 00:39
@Sovietaced
fixes
cefad53 
Signed-off-by: Jason Parraga <[email protected]>
@Sovietaced Sovietaced force-pushed the supported-tasks-cleanup branch from fa18557 to cefad53 Compare August 31, 2024 00:49
@Future-Outlier
Copy link
Member

I've built the image and test it.

image image image

Future-Outlier
Future-Outlier approved these changes Sep 3, 2024
View reviewed changes
Copy link
Member

@Future-Outlier Future-Outlier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @Sovietaced

@Future-Outlier Future-Outlier merged commit 6add5f8 into flyteorg:master Sep 3, 2024
55 checks passed
@Sovietaced Sovietaced deleted the supported-tasks-cleanup branch September 3, 2024 04:19
pmahindrakar-oss pushed a commit that referenced this pull request Sep 9, 2024
@Sovietaced @pmahindrakar-oss
Update helm/docs per changes in supported task discovery (#5694)
dbdd7e6 
* Update helm/docs per changes in supported task discovery

Signed-off-by: Jason Parraga <[email protected]>

* Cleanup flyte-binary

Signed-off-by: Jason Parraga <[email protected]>

* fixes

Signed-off-by: Jason Parraga <[email protected]>

---------

Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: pmahindrakar-oss <[email protected]>
pmahindrakar-oss pushed a commit that referenced this pull request Sep 9, 2024
@Sovietaced @pmahindrakar-oss
Update helm/docs per changes in supported task discovery (#5694)
033c743 
* Update helm/docs per changes in supported task discovery

Signed-off-by: Jason Parraga <[email protected]>

* Cleanup flyte-binary

Signed-off-by: Jason Parraga <[email protected]>

* fixes

Signed-off-by: Jason Parraga <[email protected]>

---------

Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: pmahindrakar-oss <[email protected]>
bgedik pushed a commit to bgedik/flyte that referenced this pull request Sep 12, 2024
@Sovietaced @bgedik
Update helm/docs per changes in supported task discovery (flyteorg#5694)
2dc91ca 
* Update helm/docs per changes in supported task discovery

Signed-off-by: Jason Parraga <[email protected]>

* Cleanup flyte-binary

Signed-off-by: Jason Parraga <[email protected]>

* fixes

Signed-off-by: Jason Parraga <[email protected]>

---------

Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Future-Outlier Future-Outlier Future-Outlier approved these changes

@neverett neverett Awaiting requested review from neverett

@ppiegaze ppiegaze Awaiting requested review from ppiegaze ppiegaze is a code owner

@davidmirror-ops davidmirror-ops Awaiting requested review from davidmirror-ops davidmirror-ops is a code owner

Assignees

@Future-Outlier Future-Outlier

@davidmirror-ops davidmirror-ops

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Sovietaced @neverett @Future-Outlier @davidmirror-ops